Retry APNs2 frames dropped by a mid-flight connection reset, and add structured push logging - #9

Problem

Stacked on #7. Apns2::Delivery has the same latent silent-drop pattern #7 fixed for Apnsp8 — flagged there as a deliberate follow-up rather than folded into that change:

apns2 shares the same latent silent-drop pattern, but its perform has an untested per-notification SSLError rescue that leaves notifications unmarked; applying reconciliation there needs its own analysis + specs. Deliberately scoped out to keep this change regression-safe.

Observed in production: currypizzahouse_ios (a cert-based apns2 app) went completely silent for several hours — no sends, no errors in the logs — then resumed on its own with no restart or config change. Pulling rpush_notifications for that merchant shows the same request pattern succeeding on one attempt and then producing no delivered/failed/retryable outcome at all on another, minutes apart, against the same two device tokens. That's consistent with net-http2 dropping in-flight streams on a connection reset without it ever reaching Apns2::Delivery#perform's rescue blocks.

Root cause

Same as #7: @client.join does not raise on a mid-flight connection reset. net-http2 detects it on its background socket thread and delivers the error to the client's on(:error) callback rather than re-raising into perform; #join then returns normally once the stream set empties. The in-flight notifications never receive their on(:close), so handle_response never runs for them — they're marked neither delivered, failed, nor retryable, and ensure @batch.all_processed completes the batch with them recorded nowhere.

apns2 has a second, distinct version of the same class of bug that apnsp8 doesn't: preparing a request can raise OpenSSL::SSL::SSLError before the notification ever gets a stream. The existing code:

rescue OpenSSL::SSL::SSLError => error
  log_error("Notification #{notification.id} failed with SSL error")
end

logs and moves on to the next notification — the notification is left with no outcome at all, same failure mode as the connection-reset case.

There's also a smaller, permanent-failure variant: handle_response's case has no when nil branch, so a stream that closes with no status code (should one ever surface that way rather than via the dropped-connection path) falls through to else and gets mark_failed'd — permanently — rather than retried.

Fix

Localized to the apns2 transport, reusing the read-only Batch#unresolved helper #7 already added (transport-agnostic, so nothing there needs to change):

• Apns2::Delivery#perform — after #join, reconcile: re-queue any unresolved notification as retryable instead of letting the batch discard it. No-op on the normal path where every stream reported a result.
• #handle_response — an absent status code is now a transport failure → retry, not mark_failed.
• The per-notification SSLError rescue — now retries the notification (via the same connection_lost-style path) instead of logging and dropping it.
• Errno::ECONNRESET was already in the synchronous rescue here (added by BUGS-1850. Retry after Errno::ECONNRESET #2/BUGS-1850) — unchanged.

Also in this PR: structured push logging, for parity with #7

Brings Loggable#log_push_event (added by #7) to this transport: delivered / failed / retrying events on Delivery, device token truncated. The ApnsHttp2 dispatcher's on(:error) callback is updated too — but including the error message, not just its class:

log_push_event(:connection_error, level: :error, error: "#{error.class}: #{error.message}")

This addresses @drn's open review comment on #7 (the apnsp8 dispatcher's equivalent line currently logs only error.class) before the same gap repeats here.

Why this is regression-safe

Same argument as #7: every existing APNs status outcome (200/410/400/429/500/503) is unchanged. Only the "no verdict from APNs" cases move from {silent drop, permanent fail} to {retry} — strictly safer. Batch#unresolved is read-only and shared, unmodified from #7.

Tests

Mirrors #7's apnsp8 coverage for the apns2 transport: the reconciliation sweep on #perform, the no-status #handle_response case, the new SSLError-at-prepare-time path (including that the batch continues to the next notification rather than aborting), and the structured logging format for delivered / failed / retrying, plus the dispatcher's connection_error message.

Could not run the full suite in this environment (native extension build tooling unavailable locally) — syntax-checked all four files (ruby -c) and modeled the specs directly on #7's already-passing apnsp8 equivalents, adjusting only for Apns2::Delivery's 3-arg constructor (no token provider). Relying on CI here.

Base branch

Based on rstojano/apnsp8-retry-dropped-frames (#7) to reuse Batch#unresolved and Loggable#log_push_event without redefining them. Should be re-based onto master once #7 merges — happy to do that flip myself once it lands.

Summary by CodeRabbit

• Bug Fixes

  • Improved APNs delivery reliability by retrying request-preparation, connection, and incomplete-response failures.
  • Prevented individual notification errors from interrupting remaining deliveries.
  • Improved handling of missing-status, retryable, and permanent failure responses.

• Improvements

  • Enhanced delivery and connection error logs with structured details, including retry reasons and error messages.
  • Added structured logging for successful, failed, and retried deliveries.
  • Improved log privacy by shortening device tokens.